Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking: fix config loading (fixes #11510, fixes #11559, fixes #11586) #11546

Merged
merged 53 commits into from May 10, 2019

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Mar 24, 2019

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix: fixes #11510, fixes #11559, fixes #11586
[X] Other, please explain: the refactoring of configurations

Closes #10928 because I realized that this PR does the same thing as #10928 (comment).

What changes did you make? (Give an overview)

This PR fixes three bugs along with the refactoring of configurations.
I'm sorry that this diff is huge.

All new files are in lib/cli-engine/ directory because only CLIEngine uses those.

The refactoring does the following items:

I describe file mapping of before/after the refactoring. There are details for each new file on the top of the source code.

Old New
lib/config-ops.js
(merge method)
lib/cli-engine/config-array/config-array.js
(n/a) lib/cli-engine/config-array/config-dependency.js
(n/a) lib/cli-engine/config-array/extracted-config.js
lib/config-ops.js
(pathMatchesGlobs method)
lib/cli-engine/config-array/override-tester.js
lib/config.js lib/cli-engine/cascading-config-array-factory.js
lib/config/config-file.js
lib/util/file-finder.js
lib/cli-engine/config-array-factory.js
lib/util/glob-utils.js
lib/util/glob.js
lib/cli-engine/file-enumerator.js

Therefore, appropriate tests to the moved logics are moved to new files.

And I added some @typedef comments for VSCode IntelliSense.

Is there anything you'd like reviewers to focus on?

  • Are there unintentional breaking changes?

I found an additional breaking change (as a bug fix) about plugin loading. Currently, even if a configuration has no plugins field, CLIEngine#executeOnFiles() and CLIEngine#executeOnText() can use plugin rules that the previous calls loaded. After this PR, CLIEngine#executeOnFiles() and CLIEngine#executeOnText() will report error messages consistently if plugins were not found in configuration.

Related to that, Linter#getRules() and CLIEngine#getRules() will change:

  • Linter#getRules() will return the rules, including built-in rules, user-defined rules, and only the plugin rules that the last Linter#verify() loaded.
  • CLIEngine#getRules() will return the rules, including built-in rules and only the plugin rules that the last CLIEngine#executeOnFiles() or CLIEngine#executeOnText() loaded.

I think that this change is good because formatters can access to unrelated rules (via RFC 10) before this PR and we already have a getter to get information of the last Linter#verify() call; Linter#getSourceCode().

@mysticatea mysticatea added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible do not merge This pull request should not be merged yet labels Mar 24, 2019
@mysticatea mysticatea changed the title Breaking: fix overrides settings in shareable configs overwrite extender's setting (fixes #11510) Breaking: fix overrides setting to not affect extendee files (fixes #11510) Mar 24, 2019
@mysticatea mysticatea changed the title Breaking: fix overrides setting to not affect extendee files (fixes #11510) Breaking: fix overrides setting to not affect extendee (fixes #11510) Mar 24, 2019
@mysticatea mysticatea added this to Implemented, pending review in v6.0.0 Mar 26, 2019
@mysticatea mysticatea changed the title Breaking: fix overrides setting to not affect extendee (fixes #11510) Breaking: fix bugs in config loading (fixes #11510, fixes #11559) Mar 29, 2019
@mysticatea mysticatea force-pushed the issue11510 branch 2 times, most recently from a864ae1 to 89b3292 Compare April 4, 2019 12:10
@mysticatea mysticatea changed the title Breaking: fix bugs in config loading (fixes #11510, fixes #11559) Breaking: fix config loading (fixes #11510, fixes #11559, fixes #11586) Apr 4, 2019
@platinumazure
Copy link
Member

@mysticatea PR #11388 has been merged. It also looks like this needs a rebase/merge. Once that's done, feel free to request my review or ping me and I'll review this. Thanks!

@mysticatea mysticatea force-pushed the issue11510 branch 3 times, most recently from 88e1649 to 2fc4464 Compare April 5, 2019 17:31
@mysticatea mysticatea removed the do not merge This pull request should not be merged yet label Apr 5, 2019
@mysticatea
Copy link
Member Author

Hi. I updated this PR and I think ready for review!

@mysticatea mysticatea moved this from Implemented, pending review to Ready to merge in v6.0.0 Apr 8, 2019
@mysticatea mysticatea moved this from Ready to merge to Implemented, pending review in v6.0.0 Apr 8, 2019
@mysticatea
Copy link
Member Author

I updated this PR to resolve conflicts.

@mysticatea
Copy link
Member Author

mysticatea commented Apr 9, 2019

I will revert moved files because I have found that Git didn't recognize cli-engine.js as moved.

@mysticatea
Copy link
Member Author

Thank you very much for making time to review this large PR!

I think that I fixed all the suggested changes.

@not-an-aardvark
Copy link
Member

This looks good to me after #11546 (comment) is addressed.

Also, I think this PR should remove these lines from the migration guide.

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mysticatea
Copy link
Member Author

Thank you very much for the review of this huge PR!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
No open projects
v6.0.0
  
Done
3 participants